Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race Condition and ABBA Deadlock #645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PengZheng
Copy link
Contributor

@PengZheng PengZheng commented Sep 19, 2023

Checking whether a bundle is active in celix_bundleContext_useBundles callback suffers from check-then-act race condition.
This race condition is a minor one, but immediate uninstall of a bundle after framework startup may crash the program.

@PengZheng PengZheng changed the title Remove a check-then-act race condition. hotfix/Remove a check-then-act race condition. Sep 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Merging #645 (0c2c973) into master (91ff81c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 0c2c973 differs from pull request most recent head beb910e. Consider uploading reports for the commit beb910e to get more accurate results

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
+ Coverage   81.58%   81.61%   +0.02%     
==========================================
  Files         260      260              
  Lines       34679    34678       -1     
==========================================
+ Hits        28294    28303       +9     
+ Misses       6385     6375      -10     
Files Changed Coverage Δ
libs/framework/src/dm_dependency_manager_impl.c 83.04% <100.00%> (+2.09%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@PengZheng
Copy link
Contributor Author

PengZheng commented Sep 19, 2023

This PR manifests a deadlock in test_components_ready.
More investigation is needed.

It turns out to be an insidious ABBA deadlock:

  1. When celix_componentsReadyCheck_destroy is called, ready_check's fsmMutex is locked, and celix_componentsReadyCheck_destroy will try to use the Celix event loop.
  2. When celix_componentReadyCheck_check calls celix_dependencyManager_allComponentsActive, which in turn tries to check ready_check's readiness, the Celix event loop is occupied and the current thread tries to acquire ready_check's fsmMutex.

We need to work out a fix for this deadlock before fixing the race condition. @pnoltes
A proper fix might be to exclude ready_check from readiness checking so that ready_check's fsmMutex don't have to be acquired for that purpose. For example, the following interface should allow us to deselect ready_check:

CELIX_FRAMEWORK_EXPORT size_t celix_framework_useActiveSelectedBundles(celix_framework_t* fw,
                                                                       bool includeFrameworkBundle,
                                                                       void* callbackHandle,
                                                                       bool (*select)(void* handle, const celix_bundle_t* bnd),
                                                                       void (*use)(void* handle, const celix_bundle_t* bnd));

More generally, useBundlesWithOptions could be added.

Such API changes should be postponed after 2.4.0.

The above proposed fix will not work, because the same ABBA deadlock could happen for other bundles when stopping them.

For now I'll leave this PR open as a reminder.

@PengZheng PengZheng added do-not-merge Indicates that a PR should not be merged kind/bug Categorizes issue or PR as related to a bug. labels Sep 19, 2023
@PengZheng PengZheng changed the title hotfix/Remove a check-then-act race condition. Race Condition and ABBA Deadlock Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Indicates that a PR should not be merged kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants